Skip to content

Cheetah C18 Viktoriia Zolotova & Vivian Zhu #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

viviantomato
Copy link

No description provided.

Copy link

@kendallatada kendallatada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Viktoriia and Vivian (Team V)! Your project has been scored as green.

Nice work! Let me know if you have any questions. 😊

# get letter from the copy dict/pool random.ran()
# draw one letter a time, x 10, for loop (0, 10),
# also update the qty. of the letter in each loop
LETTER_POOL = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job creating constants for the letter pool and letter scores! ✨

'Z': 10
}

def copy_dictionary(pool):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice helper function! I'd also recommend checking out the deepcopy() function. :)

letter_bank_dict = copy_dictionary(LETTER_POOL)
while len(res_letters) < 10:
letter = random.choice(list(letter_bank_dict.keys()))
if letter_bank_dict[letter] > 0:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach to drawing ten random letters absolutely works, but consider a scenario where the random.choice() function keeps selecting letters that are already at a frequency of 0. There's no telling how many times this loop may need to run! A small change we can make to guarantee our loop will only run 10 times is removing the letter from the dictionary once its frequency is at 0.

To accomplish this, we could refactor your loop to look something like this:

while len(res_letters) < 10:
    letter = random.choice(list(letter_bank_dict.keys()))
    res_letters.append(letter)
    letter_bank_dict[letter] -= 1
    if letter_bank_dict[letter] == 0:
        del letter_bank_dict[letter]

word_dict = create_dict(word)
letter_bank_dict = create_dict(letter_bank)

for letter, quantity in word_dict.items():

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job with your uses_available_letters() function! ✨

(optional) There is a solution for this function that only uses one loop and doesn't require any additional data structures to be created. See if you can optimize your space and time complexity! 🤔😌

"""
data_dict = {}
for element in data:
if element not in data_dict:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend checking out the dictionary setdefault() method. This method could help clean up this if-else block by handling situations where the dictionary key doesn't exist. ☺️

for letter in word:
if letter in SCORE_POOL.keys():
total_score += SCORE_POOL[letter]
return total_score

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job with your score_word() function! ✨

win_name = word

for word in word_list:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop header and the loop header on line 141 are identical. Consider how you can combine these two loops to prevent having to iterate over the same data twice.

#check if len of word != 10, word with fewest len win
#and if words with the same length -> pick the first one in tie_word_dict
else:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that you have implemented a for-else loop here! This structure works here, but for-else loops are really meant to be used when we're searching for values with loops. They're great for conditionally running code if a value wasn't found. You can read more about them here, but this is a more advanced topic and they really aren't used very often. So, no pressure to learn about them.

For this function, I would recommend removing the else statement and having the rest of the code directly after the for loop.

#check if len of word in tie_word_dict = 10, this word win
for word, length in tie_word_dict.items():
while length == 10:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this was a typo, but this line should be an if statement rather than a while loop. It still works because even though it's a loop, there is a return statement inside. So, when we find a word that is of length 10, we enter the loop and then immediately exit the loop and the function.

key_list = list(tie_word_dict.keys())
val_list = list(tie_word_dict.values())
win_name = key_list[val_list.index(mini_length)]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting way to find the word matching the mini_length! Be careful though because this does require creating two new data structures in memory (key_list and val_list) and the .index() method is an O(n) operation. Consider how you could track the word with the shortest length alongside mini_length in the for loop on line 164.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants